Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create a shareable ESLint configuration package #689

Merged
merged 13 commits into from
Sep 21, 2016

Conversation

fson
Copy link
Contributor

@fson fson commented Sep 20, 2016

Move the ESLint configuration to a separate package eslint-config-react-app. This allows using the same configuration in projects not using CRA. It also makes the amount of configuration in an ejected project smaller: the starter ESLint config of an ejected the project will be just {"extends": "react-app"}.

Test plan:
[x] Check that linting errors are shown correctly when running npm start.
[x] Check that it also works end to end. (Tested with a local npm server.)

(NB: the e2e test will break at the moment because the package hasn't been published yet, and it tries to install it from npm. However, I tested by publishing it to a local npm registry with sinopia, and it worked. I think we might want to come up with a way to end-to-end test all the packages together without having to manually pack all of them in the test script. But for now pulling the linter config from npm is probably good enough?)

@ghost ghost added the CLA Signed label Sep 20, 2016
@@ -0,0 +1,21 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll also want a readme here, explaining in a few words how to use this package as standalone.

"eslint-plugin-flowtype": "^2.18.1",
"eslint-plugin-import": "^1.12.0",
"eslint-plugin-jsx-a11y": "^2.2.2",
"eslint-plugin-react": "^5.2.2"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to keep dependencies pinned, and update them manually. Our setup is a little more conservative.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're at it, can you look into updating us to eslint-plugin-react@6? Have any rules changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pinned the dependencies and added a README. I can look into updating the plugins tomorrow.

Copy link
Contributor

@gaearon gaearon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a README and pin deps

1. Install this package, ESLint and the necessary plugins:

```
npm install eslint-config-react-app babel-eslint@6.1.2 eslint@3.5.0 eslint-plugin-flowtype@2.18.1 eslint-plugin-import@1.12.0 eslint-plugin-jsx-a11y@2.2.2 eslint-plugin-react@5.2.2 --save-dev
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think having to install all the dependencies separately like this is ridiculous, but until ESLint supports plugin dependencies for shareable configs, there isn't anything we can do about it, I'm afraid :(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do the same trick as here: https://github.com/airbnb/javascript/tree/master/packages/eslint-config-airbnb#eslint-config-airbnb-1. I'm just not sure what the Windows version would be like.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could write it in JS and have a node -e '<SCRIPT>' here, so it would be the same for Windows? I assume everyone who is going to use this will have node installed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think having the command that you can copy paste to your console here is better than that trick, as long as we keep the version numbers up-to-date. It's a bit more work for us, but easier for the users.

@gaearon gaearon added this to the 0.5.0 milestone Sep 21, 2016
@ghost ghost added the CLA Signed label Sep 21, 2016
useEslintrc: false
},
// @remove-on-eject-end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need this for prod config too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We copy the .eslintrc to the ejected project, so we don't need to specify a custom configFile path or to disable the .eslintrc. I think people prefer the rc files for configuration in an ejected project (see #410).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't mean ejected, I meant webpack.config.prod.js. Don't we need to update path in it as well as .dev.js?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, great point. I forgot we had ESLint for the build too. Updated now.

"eslint": "3.5.0",
"eslint-config-react-app": "file:packages/eslint-config-react-app",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, had no idea you could do that

@gaearon gaearon merged commit a2d0469 into facebook:master Sep 21, 2016
@gaearon
Copy link
Contributor

gaearon commented Sep 21, 2016

Thanks!

@fson fson deleted the eslint-config-react-app branch September 21, 2016 14:34
feiqitian pushed a commit to feiqitian/create-react-app that referenced this pull request Oct 25, 2016
* Move ESLint configuration to a separate package

* Remove the ESLint configuration, moved to eslint-config-react-app

* Update ESLint instructions

* Pin the package versions in eslint-config-react-app

* Add a README for eslint-config-react-app

* Update README.md

* Update README.md

* Update README.md

* Update README.md

* Update package.json

* Update package.json

* Update production eslint-loader config

* Add the ESLint config to devDependencies of the repo
@lock lock bot locked and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants